Safe Global Predefined Color Palettes #13598
Conversation
Swiftb0y
force-pushed
the
refactor/safe-predefinedcolorpalettes
branch
2 times, most recently
from
5963bf4
to
a5b7d98
Compare
src/library/basetracktablemodel.cpp
Outdated
| @@ -109,7 +109,7 @@ int BaseTrackTableModel::s_bpmColumnPrecision = | |||
| kBpmColumnPrecisionDefault; | |||
| bool BaseTrackTableModel::s_keyColorsEnabled = kKeyColorsEnabledDefault; | |||
| ColorPalette BaseTrackTableModel::s_keyColorPalette = | |||
| mixxx::PredefinedColorPalettes::kDefaultKeyColorPalette; | |||
| mixxx::predefinedcolorpalettes::get().defaultKeyColorPalette; | |||
|
Did you consider to make PredefinedColorPalettes a singleton? This avoids to initalize it over and over again and will avoid the new predefinedcolorpalettes namespace. |
|
It is effectively a singleton. It is only getting initialized once. see Static block variables |
This will avoid the "static initialization order fiasco" by putting all palettes behind a C++11 "magic static" that ensure that that they are initialized before they're first used.
Swiftb0y
force-pushed
the
refactor/safe-predefinedcolorpalettes
branch
from
a5b7d98
to
1c7afe6
Compare
|
fyi, an alternative implementation would be to use My current favorite is the latter (gutting out the Qt containers in favor of |
|
The ColorPalletes are currenrtly copied by value, which is a matter of three pointers due to Qt's implicit sharing. |
| @@ -146,7 +146,9 @@ void ColorPaletteEditor::initialize( | |||
| m_resetPalette = paletteName; | |||
| QString saveName = paletteName; | |||
|
|
|||
| for (const ColorPalette& palette : mixxx::PredefinedColorPalettes::kPalettes) { | |||
| const auto& kPalettes = mixxx::predefinedcolorpalettes::get(); | |||
There was a problem hiding this comment.
A free get() function in a namespace looks weird. This is an indicator that the Namespace is an object.
There was a problem hiding this comment.
well, its effectively a singleton. So its up to interpretation whether it should be modeled as a class with a static member function or a namespace with a free function. they're basically equivalent, though I find the namespace more elegant.
| kRekordboxHotcueColor16, | ||
| }; | ||
|
|
||
| const static PredefinedColorPalettes kPalettes{ |
There was a problem hiding this comment.
Returning a local static is one way ti implement a singleton pattern. How about fully make use of that here?
as in a PredefinedColorPalettes::instance() function.
| kTritKeyColor11, | ||
| kTritKeyColor12, | ||
| }}, | ||
| .defaultHotcueColorPalette = kPalettes.mixxxHotcueColorPalette, |
There was a problem hiding this comment.
This is a deep copy. Can we avoid it?
There was a problem hiding this comment.
we can simply store a const ref because we know the object outlives the reference.
| .defaultKeyColorPalette = kPalettes.mixxxKeyColorPalette, | ||
| .palettes{ | ||
| // Hotcue Color Palettes | ||
| kPalettes.mixxxHotcueColorPalette, |
There was a problem hiding this comment.
This are also deep copies, which should be avoided.
There was a problem hiding this comment.
The original were also deep copies though IIRC, but it should be easy to change.
|
summarizing your review. there are two issues:
|
As suggested (by me) in #13594 (comment) as an alternative to that PR.
Note that in case we manage to make
ColorPaletteconstexpr friendly, we can trivially migrategetto be constexpr as well without having to change the callers with this approach.